[http1 codec] Allow requests with Transfer-Encoding and Content-Length headers set.#12349
[http1 codec] Allow requests with Transfer-Encoding and Content-Length headers set.#12349mattklein123 merged 46 commits intoenvoyproxy:masterfrom
Conversation
Signed-off-by: Oleg Guba <oleg@dropbox.com>
Signed-off-by: Oleg Guba <oleg@dropbox.com>
alyssawilk
left a comment
alyssawilk
left a comment
There was a problem hiding this comment.
Cool, thanks for tackling this - I'm sure someone else down the line is going to have traffic which hits this unusual corner case.
mattklein123
left a comment
mattklein123
left a comment
There was a problem hiding this comment.
Thanks for driving this forward. A few comments.
/wait
|
/assign @antoniovicente |
Signed-off-by: Oleg Guba <oleg@dropbox.com>
|
/wait |
Signed-off-by: Oleg Guba <oleg@dropbox.com>
Signed-off-by: Oleg Guba <oleg@dropbox.com>
|
/wait |
Signed-off-by: Oleg Guba <oleg@dropbox.com>
|
/wait |
Signed-off-by: Oleg Guba <oleg@dropbox.com>
Signed-off-by: Oleg Guba <oleg@dropbox.com>
mattklein123
left a comment
mattklein123
left a comment
There was a problem hiding this comment.
Thanks, this LGTM. Will give @PiotrSikora @htuch some more time to take a look again if they want. @antoniovicente is out till Monday so all things being equal I would prefer to wait for him to get back to take a final look. cc @asraa also for the H1 codec error handling changes.
alyssawilk
left a comment
alyssawilk
left a comment
There was a problem hiding this comment.
Looks good from my end - just two minor nits.
antoniovicente
left a comment
antoniovicente
left a comment
There was a problem hiding this comment.
Looked at changes since my last set of comments. Thanks for adding tests for the response cases.
| } else if (parser_.status_code == 204 || parser_.status_code == 304 || | ||
| (parser_.status_code >= 200 && parser_.content_length == 0)) { | ||
| (parser_.status_code >= 200 && parser_.content_length == 0 && | ||
| !(parser_.flags & F_CHUNKED))) { |
There was a problem hiding this comment.
Out of curiosity: what coverage do we have for 1xx responses that include content-length or transfer-encoding: chunked headers?
…e code to use it Signed-off-by: Oleg Guba <oleg@dropbox.com>
Signed-off-by: Oleg Guba <oleg@dropbox.com>
Signed-off-by: Oleg Guba <oleg@dropbox.com>
|
/wait |
Signed-off-by: Oleg Guba <oleg@dropbox.com>
Signed-off-by: Oleg Guba <oleg@dropbox.com>
mattklein123
left a comment
mattklein123
left a comment
There was a problem hiding this comment.
Thanks LGTM. @antoniovicente @alyssawilk any further comments?
…h headers set. (envoyproxy#12349) opt-in for serving requests/responses with Content-Length and Transfer-Encoding: chunked. Per RFC remove Content-Length header before forwarding it to upstream. Signed-off-by: Oleg Guba <oleg@dropbox.com> Signed-off-by: Clara Andrew-Wani <candrewwani@gmail.com>
|
When would this PR get into envoyproxy/envoy-wasm so that istio can pick it up? We are actually hitting this edge case. @alyssawilk |
|
Up to folks on that side. |
|
@alyssawilk we won't be able to test this until this version of envoyproxy gets into istio and then into our IBM managed istio offering. |
7 participants
Commit Message: [http1 codec] Allow HTTP/1 requests/responses with Transfer-Encoding and Content-Length headers set.
Additional Description: opt-in for serving requests/responses with Content-Length and Transfer-Encoding: chunked. Per RFC remove Content-Length header before forwarding it to upstream.
Update http-parser version to include fix for nodejs/http-parser#517.
Risk Level: Medium
Testing: unit tests, update integration test
Docs Changes: updated docs/root/version_history/current.rst
Release Notes: http: added allow_chunked_length configuration option for HTTP/1 codec to allow processing requests with both Content-Length and Transfer-Encoding: chunked. If enabled - per RFC Content-Length is removed before sending to upstream.
Fixes #11398